-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: absolute positioning element blackouts in cy.screenshot #22756
Conversation
Thanks for taking the time to open a PR!
|
return before($el) | ||
// get the current body of the AUT to accurately calculate screenshot blackouts | ||
// as well as properly enable/disable CSS animations while screenshotting is happening | ||
const $body = Cypress.$('body') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there possibly a cleaner way to reference the body here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prob isn't what you are looking for, but I think we store the AUT frame on the cypress instance
$autIframe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine - since the Cypress
instance is injected to the AUT, and you can only have one instance of HTMLBodyElement
(or the browser will raise an error) this seems safe to me.
blackout: ['.blue'], | ||
onBeforeScreenshot () { | ||
const blackedOutElementCoordinates = Cypress.$( | ||
'#__cypress-animation-disabler+div.__cypress-blackout', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to better selectors here. This seems to be a bit of a coincidence that our blacked out elements are adjacent to the animation disabler element. Also the test assertions below aren't great, but at least should guarantee that the blacked out element is very close to the thing it's trying to black out.
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit around types - probably a quick win, but going to approve.
@@ -337,7 +337,7 @@ const takeScreenshot = (Cypress, state, screenshotConfig, options: TakeScreensho | |||
} | |||
} | |||
|
|||
const before = ($el) => { | |||
const before = ($body, $container) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we incrementally ad types as we go? HTMLBodyElement
and HTMLElement
, in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to create a follow up PR to add types to the screenshot files since it's quite a bit of changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened #22768 against this branch and will rebase it once I merge this in
return before($el) | ||
// get the current body of the AUT to accurately calculate screenshot blackouts | ||
// as well as properly enable/disable CSS animations while screenshotting is happening | ||
const $body = Cypress.$('body') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine - since the Cypress
instance is injected to the AUT, and you can only have one instance of HTMLBodyElement
(or the browser will raise an error) this seems safe to me.
User facing changelog
Fixes a regression in 9.6 where absolute elements that are blacked out with
cy.screenshot
are positioned off the container element and not the body.Additional details
This PR attempts to fix a regression when screenshots were refactored for
cy.origin
in #20949 where element dimensions are calculated from the screenshotted element. This works in most cases, but causes some trouble with absolute elements. To fix this, since we no longer have access to theAutIframe
class, is to reference the body to calculate dimensions for elements.Steps to test
See changes to the
screenshot_viewport_capture_spec.js
How has the user experience changed?
Before code changes with added regression test in
screenshot_viewport_capture_spec.js
After code changes with added regression test in
screenshot_viewport_capture_spec.js
PR Tasks
cypress-documentation
?type definitions
?